- 
                Notifications
    
You must be signed in to change notification settings  - Fork 739
 
feat: prevent Electron versions that are in use by some window from being removed #1395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
116874c    to
    4398a4e      
    Compare
  
    | 
           I've been pondering it some more, and I think we should expand the usage of the locking as a general solution. There's two main things which need to be solved regarding versions and multiple windows: 
 The current state of this PR is solving 2, but I think we can solve 1 at the same time. Here's what I've been pondering: 
  | 
    
4398a4e    to
    96a196b      
    Compare
  
    | 
           I did what you suggested and came up with a way to simulate a user trying to do something simultaneously in multiple windows. It's a bit convoluted but the results seem to be reliable, so here it goes: For the  
 const receiverBroadcastChannel = new BroadcastChannel('testLocks');
receiverBroadcastChannel.addEventListener('message', async () => {
  // the version is actually removed if we don't wait for a bit, but I assume
  // users doing it manually would be at least this slow anyway :)  
  await new Promise(resolve => setTimeout(resolve, 20))
  console.log('trying to remove the version the other window just selected');
  const v25_3_0_removeButton = [
    ...document.querySelectorAll('table .bp3-button'),
  ][1];
    
  v25_3_0_removeButton.click();
});
 // $0 is the button that selects v25.3.0
$0.click();
const emitterBroadcastChannel = new BroadcastChannel('testLocks');
emitterBroadcastChannel.postMessage('whatever');The first window will simulate the user clicking on the "Remove" button for 25.3.0 just after that version is selected in the second window, and nothing happens: 2023-07-24_23-25-46.mp4For the simultaneous downloads case (video below): 
 const receiverBroadcastChannel = new BroadcastChannel('testLocks');
receiverBroadcastChannel.addEventListener('message', () => {
  console.log('trying to download...');
  document.querySelector('a[label="Not downloaded"]').click();
});
 const emitterBroadcastChannel = new BroadcastChannel('testLocks');
emitterBroadcastChannel.postMessage('whatever');Both windows will receive the broadcast at the same time. Only one of the windows will be granted the lock at first (the window on the right side in the video), and the other window will only get the lock when the download finishes in the first window. Even though the lock does its job, we still get an error and end up with an inconsistent state ("Checking status" in the download selector in the window that first got the lock, and a "dest already exists" error in the other window) when both windows try to download the same version in rapid succession (the same thing happens in the case you mentioned of the user opening a new window when a download is in progress), so maybe  2023-07-24_23-32-14.mp4I think this is mostly ready for review, but I'd like some feedback on this before I add some new tests 🙂  | 
    
ec48598    to
    fd6d0f4      
    Compare
  
    
Some initial work on #1394:
2023-07-09_22-25-22.mp4
A couple call-outs:
navigator.locks@dsanders11 I used plain lifecycle methods to update the UI when a version is set as active in another window, so we might be able to do without another dependency here if that aligns with what you had in mind.
ElectronSettingsis unmounted because of the async state update (there's a comment in the code with more details).Fixes #1394.